Randomize chain source selection in tests #769
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
|
This will be flaky, at the very least until lightningdevkit/rust-lightning#4341 gets release, which would have been caught if we'd ever had run the 0conf test case with an Electrum chain source. |
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
makes sense to me. Hopefully CI fails until lightningdevkit/rust-lightning#4341
I think I'll whack-a-mole a few more flakes/bugs before landing this. |
|
Previously had failed, I want to double-check them once more before moving forward here. |
tnull
force-pushed
the
2026-01-test-setup
branch
from
96b6f29 to
b8179ca
Compare
|
Pushed an update, but currently |
tnull
force-pushed
the
2026-01-test-setup
branch
from
b8179ca to
282605b
Compare
It's weird to have a special intermediary `setup_node` method if we have `TestConfig` for exactly that reason by now. So we move `async_payment_role` over.
tnull
force-pushed
the
2026-01-test-setup
branch
from
7796735 to
d0946d1
Compare
Alright, fixed that one, too and rebased. Should be ready for review now. |
.. all of our tests should be robust against switching chain sources. We here opt to pick a random one each time to considerably extend our test coverage, instead of just running some cases against non-Esplora chain sources. Signed-off-by: Elias Rohrer <[email protected]>
When we intially implemented `bitcoind` syncing polling the mempool was very frequent and rather inefficient so we made a choice not to unnecessarily update the payment store for mempool changes, especially since we only consider transactions `Succeeded` after `ANTI_REORG_DELAY` anyways. However, since then we made quite a few peformance improvements to the mempool syncing, and by now we should just update they payment store as not doing so will lead to rather unexpected behavior, making some tests fail for `TestChainSource::Bitcoind`, e.g., `channel_full_cycle_0conf`, which we fix here. As we recently switched to updating the payment store based on BDK's `WalletEvent`, but they currently don't offer an API returning such events when applying mempool transactions, we copy over the respective method for generating events from `bdk_wallet`, with the intention of dropping it again once they do. Signed-off-by: Elias Rohrer <[email protected]>
Previously, we fixed than a fresh node syncing via `bitcoind` RPC would resync all chain data back to genesis. However, while introducing a wallet birthday is great, it disallowed discovery of historical funds if a wallet would be imported from seed. Here, we add a recovery mode flag to the builder that explictly allows to re-enable resyncing from genesis in such a scenario. Going forward, we intend to reuse that API for an upcoming Lightning recoery flow, too.
Previously, we'd selectively insert the funding outputs into the onchain wallet to later allow calculating `fees_paid` when creating payment store entries (only for splicing mostly). However, this didn't always work, and we might for example end up with a missing funding output (and hence would fall back to `fees_paid: Some(0)`) if it was a counterparty-initiated channel and we synced via `bitcoind` RPC. Here, we fix this by tracking all LDK-registered `txids` in `ChainSource` and then in the `Wallet`'s `Listen` implementation insert all outputs of all registered transactions into the `Wallet`, ensuring we'd always have sufficient data for `calculate_fee` available. Thereby we also fix the `onchain_send_receive` test which previously failed when using `TestChainSource::Bitcoind`. Signed-off-by: Elias Rohrer <[email protected]>
tnull
force-pushed
the
2026-01-test-setup
branch
from
d0946d1 to
7c08a0b
Compare
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
I don't really know BDK well enough to feel like I can meaningfully review this, but a few questions.
|
|
||
| // FIXME/TODO: This is copied-over from bdk_wallet and only used to generate `WalletEvent`s after | ||
| // applying mempool transactions. We should drop this when BDK offers to generate events for | ||
| // mempool transactions natively. |
There was a problem hiding this comment.
Hmm, if BDK doesn't support this natively, do they have logic to correctly handle conflicts and build balance knowledge appropriately? ie if there's an RBF that conflicts do we remove the 0conf tx from our balance?
| /// | ||
| /// This should only be set on first startup when importing an older wallet from a previously | ||
| /// used [`NodeEntropy`]. | ||
| pub fn set_wallet_recovery_mode(&mut self) -> &mut Self { |
There was a problem hiding this comment.
Rather than a coarse "recovery mode" can we just enable setting the "wallet birthday"? In some setups that might be available and would avoid a lot of effort.
3 participants
.. all of our tests should be robust against switching chain sources. We
here opt to pick a random one each time to considerably extend our test
coverage, instead of just running some cases against non-Esplora chain
sources.